fix: When the function parameter is of non string type, if the parameter is empty, the function will run with an error #3053#3107
Conversation
…ter is empty, the function will run with an error #3053
|
Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
| if not is_required and (value is None or (isinstance(value, str) and len(value) == 0)): | ||
| return None | ||
| if not is_required and source == 'reference' and (value is None or len(value) == 0): | ||
| return None |
There was a problem hiding this comment.
The provided code has a few concerns:
-
Redundant Check: The second
ifstatement checks both ifsourceis'reference'and if the value is empty. This redundancy can be resolved by checking only one condition. -
String Length Consideration: The current implementation does not explicitly handle cases where the input could contain non-string values that should also be considered "empty," although it implicitly handles them because
Noneis handled separately. -
Immutability of List: Although this doesn't appear to cause an issue with the given code, lists cannot be used immutably in certain contexts without explicit conversion (e.g., using tuple assignment).
Here's a revised version with these considerations addressed:
def valid_reference_value(_type, value, name):
def convert_value(name: str, value, _type, is_required, source, node):
if value is None and is_required:
raise ValueError(f"Required field '{name}' missing.")
# Convert string to title case unless required specifically
if _type == 'title-cased':
if isinstance(value, str) and len(value.strip()) > 0:
return value.title()
# Handle null or blank values appropriately based on requirement and source
if not is_required and (value is None or
(isinstance(value, str) and len(value.strip()) == 0)):
return NoneKey Changes:
- Removed redundant check inside the second
if. - Added logic for converting strings to title case when
_typeis'title-cased'and there is no content. - Raised a
ValueErrorfor required fields that areNone.
These changes help improve readability, error handling, and potentially performance depending on context.
| if not is_required and (value is None or (isinstance(value, str) and len(value) == 0)): | ||
| return None | ||
| if source == 'reference': | ||
| value = node.workflow_manage.get_reference_field( |
There was a problem hiding this comment.
The provided function convert_value has a minor issue with handling empty string conversions from references:
Issue: The current check (isinstance(value, str) and len(value) == 0) will always result in False, because both conditions must be True. If the condition fails at all (None), the outer check will automatically handle it.
Suggestion:
Remove the first condition that checks for an empty string when the value is already None.
Here's the updated code snippet:
@@ -49,7 +49,6 @@ def valid_reference_value(_type, value, name):
def convert_value(name: str, value, _type, is_required, source, node):
if not is_required and value is None:
return None
# Check if source is reference
if source == 'reference':
# Get value using workflow_manage.get_reference_field methodThis ensures that the conversion logic only executes if the value is not None.
fix: When the function parameter is of non string type, if the parameter is empty, the function will run with an error #3053